fix(engine): report single-keyframe videos as sparse GOP - #3573

Reviewed at cf5e5042865a6099c56da513395451ffe92f56f8.

This one is correct and I could not find a defect in it.

The bug is real: the old timestamps.length < 2 branch returned isProblematic: false unconditionally, so a single-keyframe file of any duration was reported as healthy. A file with one keyframe is one GOP spanning the whole stream, so treating the duration as the effective interval is the right model.

Checks I ran rather than assumed:

• runFfprobe exists in this file and the two new calls match its signature.
• Rounding is consistent with the multi-keyframe path, which also uses Math.round(x * 100) / 100 on both interval fields.
• The > 2 threshold matches the existing maxInterval > 2 used by the multi-keyframe return, so a single-keyframe file is judged on the same rule rather than a new one.
• Splitting the old combined branch into an explicit length === 0 case preserves the previous zero-keyframe behavior exactly.
• probeStreamDurationSeconds degrades correctly: stream probe, then format probe, then 0, and 0 > 2 is false, so a total probe failure reverts to the old not-problematic answer rather than a false alarm.

The extra ffprobe invocation only occurs on the single-keyframe path, which is the rare case, so the cost is well placed.

Not approving only because this PR is bot-authored and my standing rule is that a bot's autonomously created PR needs the owner's explicit go before I stamp it. On the code itself I have no objection.

Review by Rames

Conflict note: #3576 changes this same function from the same base blob c4358494e1, returning isProblematic: true unconditionally with zeroed intervals. That version flags healthy short clips and contradicts the file's own > 2 threshold. This PR is the one to keep; I have recommended closing #3576.

Reviewed at cf5e5042865a6099c56da513395451ffe92f56f8. The code is sound. I am not approving yet only because CI is red, and the cause is mechanical.

The fix is correct

Splitting the old timestamps.length < 2 guard into a zero case and a one case is the right shape. Zero keyframes means the probe told us nothing, so reporting isProblematic: false is the honest answer. One keyframe is a real measurement: the GOP spans the whole stream, so the interval to report is the duration, and isProblematic: duration > 2 lands on the same threshold the multi-keyframe path uses for maxInterval. The two paths cannot disagree about what "sparse" means, which is what matters here.

probeStreamDurationSeconds degrades the way it should. Stream-level duration first, format-level second, 0 if both fail, and 0 > 2 is false, so an unprobeable file lands back on the old behaviour rather than being reported as problematic on no evidence. Both probes are guarded.

What is actually blocking: formatting, and it cascades

All four red checks trace to one cause. format:check reports issues in packages/engine/src/utils/ffprobe.ts and packages/engine/src/utils/ffprobe.test.ts. That fails Format and Preflight (lint + format), the regression shards are then skipped, and the regression gate reads skipped != success and fails too. Lint itself is clean: the Preflight log says Found 0 warnings and 0 errors immediately before the format step.

So running the formatter without --check on those two files should turn all four green. Nothing here needs a code change.

Push that and I will approve at the new head.

Nit, genuinely optional

The one-keyframe branch rounds to two decimals with Math.round(duration * 100) / 100 while the multi-keyframe branch returns its average unrounded. Harmless, just two representations of the same field.

Review by Rames

Re-review at f9e1a6668d0e69d3a39d1c20fa17377a889fca68.

The code is unchanged from the head I already reviewed, and it is still correct.

The follow-up commit is formatting only, measured rather than assumed

f9e1a666 touches ffprobe.ts and ffprobe.test.ts. I normalized both files at both heads (all whitespace removed, plus the trailing commas Prettier drops when it joins a multi-line argument list onto one line) and hashed the result:

Identical in both files. Every finding from my review at cf5e5042 therefore carries forward unchanged, and there is no new logic to audit.

Worth stating that my first attempt at this check only collapsed whitespace, did not account for that trailing comma, and reported a difference which is not there. The table above is the corrected measurement.

Correcting my own earlier framing

My previous review said I was holding approval because CI was red. That was the wrong reason to give and I withdraw it. A red format check is not a code defect, and the code merits are what I should have been answering.

On the merits this is right. The single-keyframe branch measures a real GOP spanning the whole stream, reports the duration as the effective interval, and judges it on the same > 2 threshold the multi-keyframe path already uses, so the two paths cannot disagree about what "sparse" means. probeStreamDurationSeconds degrades to 0 when both probes fail, and 0 > 2 is false, so an unprobeable file reverts to the old not-problematic answer instead of raising a false alarm.

Why this is a comment and not an approval

Unchanged from my first review, and it is not about the code. This PR is bot-authored, and my standing rule is that a bot's autonomously created PR needs the owner's explicit go before I stamp it.

Auto-merge is also armed here (miga-heygen, squash). With REVIEW_REQUIRED outstanding, an approval from me would not read as a review signal, it would be the merge trigger. That call belongs to a human on the team.

The nit from last time still stands and is still optional: the one-keyframe branch rounds to two decimals while the multi-keyframe branch returns its average unrounded.

Review by Rames